-
Notifications
You must be signed in to change notification settings - Fork 706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Super Mario 64: Rework logic for 100 Coins #4131
Conversation
As someone who plays with 100 coin stars off, I am personally against this change. Vanilla 100 coins stars mean they're still in logic, so I'm now required to do them. This change takes that away and forces 100 stars into the pool for all sm64 games. If someone wanted 100 coin stars to be vanilla, I would argue they could do that today, with plando. So while this PR makes doing a thing that could already be done easier, it takes an existing option for the rando away. I wouldn't be against having 3 options for 100 coin stars (randomized, vanilla, disabled), but at the very least I want to keep the option to have 100 coin stars simply be disabled. Obviously this is @N00byKing 's call as world maintainer, but wanted to offer my two cents as a frequent player of sm64 in ap. |
This change really doesn't seem in the spirit of the option. People leave 100 coin stars off so they don't have to do them, not because they want them to be unrandomized. Maybe a better change here would be to rename the option to "remove 100 coin stars" and reverse the values so that it's clearer what it does. If N00byKing separately sees value in this option, it could be changed to a choice option like this (Edit: This is also what remyjette suggested, I didn't read their comment first):
Obviously I don't speak for N00byKing but I would highly question the removal of the ability to completely remove 100 coin stars. |
I don't mind it being a separate option as described by @NewSoupVi and @remyjette, but replacing current behavior is definitely unwanted. |
Reworked the logic based on feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that this is optional and all current behavior is retained, I will leave the rest of this up to N00byKing.
In terms of the actual code, I have two comments but these actually also go more in the direction of @N00byKing. I have some issues with the code, but josephwhite matched the style of the rest of the apworld here, so the comments are "out of scope" for this PR and these things don't actually need to be changed for me to approve it.
I'm not sure how much SM64 is still being worked on - If it is still getting content updates, I would like if a future PR could be opened for these changes, or they could be included with a content update PR. It's not like, pressing, though, so only if you were planning on working on this apworld more anyway, or if someone else volunteers to do it.
worlds/sm64ex/Options.py
Outdated
option_Off = 0 | ||
option_On = 1 | ||
option_Vanilla = 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, this apworld uses this casing for options, huh? That's odd, it's generally supposed to be written as option_off
. But the rest of the options are like this too, so...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Would have preferred to stick with the casing used in the file to be consistent, but Options API.md does use snake case in its examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Matching existing style seems good to me, and style inconsistency isn't really something that super duper needs to be fixed.
As you said, if/when a content update comes, it may be nice to roll that in.
I personally have no plans right now to do so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes LGTM. One small comment on option comparisons but otherwise this seems good. Tested some generations and the code doesn't have any evaluations of enable_coin_stars that would be thrown off by the option changes here.
Co-authored-by: Exempt-Medic <[email protected]>
I just looked at the code again and realized something. This effectively raises the total star count by 15, since it creates additional stars to put on the 100 coin locations. If this is intentional in its current state, I feel like it should probably be documented in the tooltip To me, it would make more sense if either:
|
Co-authored-by: Exempt-Medic <[email protected]>
Co-authored-by: Exempt-Medic <[email protected]>
What is this fixing or adding?
This changes aims at refactoring the logic behind 100 Coin Stars to follow the same conventions as 1Up Boxes and Bob-omb Buddies.
generate_basic
function.Also removes the logic that having 100 Coin Stars disabled would reduce the max star count.
How was this tested?
Generated several seeds using YAMLs for SM64 (and one other game for multi-world test cases). Looked for the following behaviors:
Also ran repo-wide unit tests.